fix(executor): guard against missing content in provider responses#3952
fix(executor): guard against missing content in provider responses#3952lawrence3699 wants to merge 1 commit intosimstudioai:mainfrom
Conversation
When an LLM provider returns a 200 response without a content field, the router and evaluator handlers crash with a TypeError trying to call .trim() on undefined. This happens when a model is unavailable, rate-limited mid-stream, or returns an unexpected response structure. Add an explicit check for result.content before accessing it, matching the existing pattern in shared/response-format.ts (line 91) which already uses `result.content ?? ''` for this case. Affected handlers: - RouterBlockHandler (legacy path, line 127) - RouterBlockHandler V2 (line 287 fallback after JSON parse failure) - EvaluatorBlockHandler (line 148) Added regression tests for all three paths.
PR SummaryLow Risk Overview Adds unit tests covering missing/empty provider Reviewed by Cursor Bugbot for commit 177c6a5. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThis PR adds defensive Key changes:
Confidence Score: 5/5Safe to merge — targeted defensive fix with no happy-path logic changes and good test coverage. No P0 or P1 issues found. The single minor gap is that the router V2 test suite does not include an empty-string-content case (the legacy test suite does), but this is a P2 coverage observation and does not affect correctness. The fix is minimal, consistent with established codebase patterns, and the three new guard code paths are each covered by at least one test. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant W as Workflow Engine
participant H as Handler (Router / Evaluator)
participant P as Provider API
W->>H: execute(ctx, block, inputs)
H->>P: POST /api/providers
alt Provider returns no content field
P-->>H: 200 OK { model, tokens } (content absent)
Note over H: NEW: if (!result.content) guard
H-->>W: throw Error("Provider returned an empty response...")
Note over W: Workflow fails with actionable error
else Provider returns valid content
P-->>H: 200 OK { content: "...", model, tokens }
H->>H: trim / parse / extractJSONFromResponse
H-->>W: BlockOutput
end
Reviews (1): Last reviewed commit: "fix(executor): guard against missing con..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
This PR hardens executor handlers against provider responses that succeed (HTTP 200) but omit the expected content field, replacing runtime TypeError crashes with a descriptive error.
Changes:
- Added explicit guards in Router (legacy + V2) and Evaluator handlers before using
result.content. - Added/expanded unit tests to cover missing/empty provider
contentfor Router and Evaluator handlers.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/sim/executor/handlers/router/router-handler.ts | Adds result.content presence checks before trimming/parsing routing decisions. |
| apps/sim/executor/handlers/router/router-handler.test.ts | Adds tests for missing/empty content in legacy Router and missing content in Router V2. |
| apps/sim/executor/handlers/evaluator/evaluator-handler.ts | Adds result.content presence check before JSON extraction/parsing. |
| apps/sim/executor/handlers/evaluator/evaluator-handler.test.ts | Adds a test ensuring a clear error is thrown when content is missing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!result.content) { | ||
| throw new Error( | ||
| 'Provider returned an empty response. The model may be unavailable or the request was malformed.' | ||
| ) | ||
| } | ||
|
|
||
| const chosenBlockId = result.content.trim().toLowerCase() |
There was a problem hiding this comment.
The guard if (!result.content) only catches missing/empty values, but it will still allow truthy non-string content (e.g., an object/array) through and then result.content.trim() will throw. It also treats whitespace-only strings as non-empty, leading to a later Invalid routing decision: error instead of the intended clear provider-empty error. Consider validating result is an object and typeof result.content === 'string' and that result.content.trim().length > 0 before calling .trim()/.toLowerCase().
| if (!result.content) { | |
| throw new Error( | |
| 'Provider returned an empty response. The model may be unavailable or the request was malformed.' | |
| ) | |
| } | |
| const chosenBlockId = result.content.trim().toLowerCase() | |
| if (!result || typeof result !== 'object' || typeof result.content !== 'string') { | |
| throw new Error( | |
| 'Provider returned an empty response. The model may be unavailable or the request was malformed.' | |
| ) | |
| } | |
| const normalizedContent = result.content.trim() | |
| if (normalizedContent.length === 0) { | |
| throw new Error( | |
| 'Provider returned an empty response. The model may be unavailable or the request was malformed.' | |
| ) | |
| } | |
| const chosenBlockId = normalizedContent.toLowerCase() |
| const result = await response.json() | ||
|
|
||
| if (!result.content) { | ||
| throw new Error( | ||
| 'Provider returned an empty response. The model may be unavailable or the request was malformed.' | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
Same issue as the legacy path: if (!result.content) doesn’t protect against truthy non-string values, and whitespace-only content will fall through to the JSON parse / fallback and eventually call .trim() anyway. Tighten the validation to require a non-empty trimmed string (and ideally guard result itself) before using result.content.
| const result = await response.json() | ||
|
|
||
| if (!result.content) { | ||
| throw new Error( | ||
| 'Provider returned an empty response. The model may be unavailable or the request was malformed.' | ||
| ) | ||
| } | ||
|
|
||
| const parsedContent = this.extractJSONFromResponse(result.content) |
There was a problem hiding this comment.
if (!result.content) won’t catch truthy non-string content, so extractJSONFromResponse(result.content) can still receive a non-string and crash when it calls .trim(). Also, whitespace-only strings slip through and will likely produce a confusing downstream parse/score behavior. Consider validating typeof result.content === 'string' and result.content.trim() before proceeding.
Problem
While reading through the executor handlers, I noticed that the router and evaluator handlers access
result.contentdirectly afterresponse.json()without checking whether the field is present. If an LLM provider returns a 200 response but without acontentfield — for example when a model is temporarily unavailable, rate-limited mid-stream, or the provider returns an unexpected response structure — the handlers crash with:This surfaces as an opaque internal error to the user, with no indication of what went wrong.
Affected paths
result.content.trim().toLowerCase()at the routing decision stepresult.content.trim()in the JSON parse fallback path — thetryblock catches the parse error, but thecatchfallback also crashes if content is missingthis.extractJSONFromResponse(result.content)passes undefined intoextractJSONFromResponse, which then calls.trim()on itExisting correct pattern
shared/response-format.ts(line 91) already handles this case:The mothership handler also uses
result.content || ''. The router and evaluator handlers were the only ones missing this guard.Fix
Add an explicit
!result.contentcheck before accessing the field. When the content is missing or empty, the handler now throws a descriptive error message instead of an opaque TypeError.Before → After
Before: Provider returns
{ model: "gpt-4o", tokens: {...} }(no content field) →TypeError: Cannot read properties of undefined (reading 'trim')→ workflow fails with cryptic errorAfter: Same response →
Error: Provider returned an empty response. The model may be unavailable or the request was malformed.→ workflow fails with actionable error messageTests added
router-handler.test.ts: 3 new tests covering legacy (missing content, empty string content) and V2 (missing content)evaluator-handler.test.ts: 1 new test for missing contentAll 33 handler tests pass.